Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(npm): set up node_modules/.bin/ entries for package that provide bin entrypoints #23496

Merged

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Apr 22, 2024

Closes #23036

TODO:

  • Upgrade deno_npm and eszip
  • Add Windows support
  • Add tests (clashing binary name, explicit bin names, implicit bin name)

bartlomieju added a commit to denoland/eszip that referenced this pull request Apr 23, 2024
@bartlomieju bartlomieju changed the title [WIP] fix(npm): set up node_modules/.bin/ entries for package that provide bin entrypoints fix(npm): set up node_modules/.bin/ entries for package that provide bin entrypoints May 9, 2024
@bartlomieju bartlomieju requested a review from dsherret May 22, 2024 12:03
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM

if link.exists() {
let resolved = std::fs::read_link(&link).ok();
if let Some(resolved) = resolved {
if resolved != original {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add to the spec test a step that would cause this codepath to be hit on an already setup node_modules directory?

@nathanwhit nathanwhit enabled auto-merge (squash) May 23, 2024 21:39
@nathanwhit nathanwhit force-pushed the generate_node_modules_bin_entries branch from 560f083 to 123e428 Compare May 23, 2024 23:19
@nathanwhit nathanwhit merged commit 92a8d09 into denoland:main May 23, 2024
17 checks passed
@bartlomieju bartlomieju deleted the generate_node_modules_bin_entries branch May 24, 2024 06:24
@birkskyum birkskyum mentioned this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support running scripts that contain npm run with deno task
3 participants